fix: peerDAS - Fix name of custody_group_count in Fulu metadata#7751
fix: peerDAS - Fix name of custody_group_count in Fulu metadata#7751nflaig merged 3 commits intoChainSafe:peerDASfrom
custody_group_count in Fulu metadata#7751Conversation
| discoveryAddresses: ArrayOf(stringType), | ||
| /** Based on Ethereum Consensus [Metadata object](https://github.com/ethereum/consensus-specs/blob/v1.1.10/specs/phase0/p2p-interface.md#metadata) */ | ||
| metadata: ssz.altair.Metadata, | ||
| metadata: ssz.fulu.Metadata, |
There was a problem hiding this comment.
this is a breaking change in a sense that if the beacon node does not return custody_group_count field we will throw an error... the problem here is the api design, usually we want a new version of the api if we add new fields, I hinted at this here ethereum/beacon-APIs#489 but since we did the same for altair I guess we are ok, I doubt anyone is using our api client to query this api and since we are merging this into peerDAS branch only for now I am fine with this, but let's keep an eye on this before we merge into unstable, ideally at this point all bns should return the field
There was a problem hiding this comment.
looks like this is already an issue in our sim tests because the Lighthouse version we run there doesn't support it
There was a problem hiding this comment.
Is there a better way to differentiate between the beacon-api types and the consensus-spec types when parsing them in those tests? It seems like the beacon API types are subtly different in that they don't require syncnets and custody_group_count, unlike the consensus-spec. I agree though that'd it'd definitely make it easier to match the types in both places
There was a problem hiding this comment.
so the problem is that there is no ssz type to represent an optional field, even Optional in ssz means <type> | null, so if the field is omitted we always will have an issue
the alternative is to use ssz.altair.Metadata and handle custody_group_count outside of the ssz container, it's a bit hacky but I think we did this before somewhere, can try to find that
we usually try to use ssz containers but since the api can't support it anyways the hacky alternative would be fine
There was a problem hiding this comment.
I added a workaround, it's not pretty but should do the job for now
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## peerDAS #7751 +/- ##
==========================================
Coverage ? 42.47%
==========================================
Files ? 732
Lines ? 52922
Branches ? 2263
==========================================
Hits ? 22481
Misses ? 30399
Partials ? 42 🚀 New features to boost your workflow:
|
matthewkeil
left a comment
matthewkeil
left a comment
There was a problem hiding this comment.
EXCELLENT FIND!!!!
LGTM!!! 🥷
|
🎉 This PR is included in v1.34.0 🎉 |
4 participants
Motivation
I was updating the beacon API to include custody group count when I noticed that the metadata field should be named
custody_group_count, notcgc(unlike the ENR).Description
/eth/v1/node/identityto return Fulu metadatacgctocustodyGroupCountin Metadata